Skip to content
This repository has been archived by the owner on Feb 5, 2024. It is now read-only.

Fix access level violation #17

Merged
merged 1 commit into from Sep 26, 2012
Merged

Fix access level violation #17

merged 1 commit into from Sep 26, 2012

Conversation

saem
Copy link
Contributor

@saem saem commented Sep 25, 2012

The getLoader interface has changed and this causes immediate breakage (even upon something simple like php app/console).

I've updated it here, fixed up the doc comment (now correctly indicates that this will immediately throw an exception).

* @param ObjectManager $manager
* @param array $options
* @return EntityLoaderInterface
* @param \Doctrine\Common\Persistence\ObjectManager $manager
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the short class name in the phpdoc here (as done previously)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used PHPStorm, and it automatically generated that -- sucks because it didn't notice the use statements. I imagine this would also effect code completion for other IDEs, but I can short name the ObjectManager, and the FormException, but I have to put in a use stateument to shorten the EntityLoaderInterface for it to be resolve correctly.

I'm not sure what the style guide is here, should I short name everything, and put in the appropriate use statements? Or only shorten the items that have use statements and remain resolvable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saem In Symfony2, @fabpot prefers keeping the short name everywhere and adding the needed use statement for this. I don't know what @beberlei prefers for this bundle though. But in any case, it should be resolvable (which was indeed not the case here previously)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And btw, PhpStorm 5 handles use statements properly when resolving the phpdoc, even if it generates it with the FQCN.

The getLoader interface has changed and this causes immediate breakage (even upon something simple like php app/console).

I've updated it here, fixed up the doc comment (now correctly indicates that this will immediately throw an exception).
@saem
Copy link
Contributor Author

saem commented Sep 25, 2012

Redone, using all short names.

@stof
Copy link
Member

stof commented Sep 26, 2012

Actually, I have merge rights on this repo so let's merge :)

stof added a commit that referenced this pull request Sep 26, 2012
Fix access level violation
Closes #16
@stof stof merged commit 1967e38 into doctrine:master Sep 26, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants